Fix collision between sofdep unloading and hard symbol dependency by yevgeny-shnaidman · Pull Request #1301 · kubernetes-sigs/kernel-module-management

yevgeny-shnaidman · 2026-07-01T19:45:01Z

scenario:
1.KMM Modules needs to load module_a, module_b, module_c
2. module_a has a hard symbol dependency on module_b.
3. modulesLoadingOrder defined as: [module_a, module_b, module_c]

There is no problems when loading. When unloading using softdep, the following happens

module_a is unloaded first
module_b is unloaded automatically, due to hard symbol dependency
modprobe looks at softdep, sees that module_b is unloaded, and does not continue to module_c

This commit changes the Unload flow:

no softdep is created.
modprobe -r ocmmand is provided with the list of modules defined in the modulesLoadingOrder

This way it will go over every module in the command and try to unload it

netlify · 2026-07-01T19:45:07Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`9dc420d`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-kmm/deploys/6a4635660938fe00087804b4
😎 Deploy Preview	https://deploy-preview-1301--kubernetes-sigs-kmm.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

kubernetes-prow · 2026-07-01T19:45:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yevgeny-shnaidman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yevgeny-shnaidman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yevgeny-shnaidman · 2026-07-01T19:45:28Z

/assign @ybettan
/assign @TomerNewman

codecov-commenter · 2026-07-01T19:48:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.78%. Comparing base (fa23a9b) to head (9dc420d).
⚠️ Report is 392 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1301      +/-   ##
==========================================
- Coverage   79.09%   73.78%   -5.31%     
==========================================
  Files          51       68      +17     
  Lines        5109     5116       +7     
==========================================
- Hits         4041     3775     -266     
- Misses        882     1169     +287     
+ Partials      186      172      -14

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

TomerNewman

This way it will go over every module in the command and try to unload it

but if modprobe will do now modprobe -r a b c and when it tries to remove a, b will be removed too, no? so when modprobe will try to remove b it is already gone and because of that fail.

yevgeny-shnaidman · 2026-07-02T06:44:38Z

This way it will go over every module in the command and try to unload it

but if modprobe will do now modprobe -r a b c and when it tries to remove a, b will be removed too, no? so when modprobe will try to remove b it is already gone and because of that fail.

in this case modprobe just goes over every module it received in the command line and tries to remove, no matter if it is present or not. In case of softdep the flow is different

TomerNewman · 2026-07-02T06:26:57Z

+		}
 	}

 	w.logger.Info("Unloading module", "name", moduleName)


if we already remove multiple modules, the logs will still show that we remove only a single module

TomerNewman · 2026-07-02T08:36:36Z

+	w.logger.Info("Starting unloading", "args", args)

 	if err := w.mr.Run(ctx, args...); err != nil {
 		return fmt.Errorf("could not unload module %s: %v", moduleName, err)


I would also change the log message here since we can unload multiple modules

TomerNewman · 2026-07-02T08:39:17Z

 		}
-
-		args = append(args, moduleName)
+		if len(cfg.Modprobe.ModulesLoadingOrder) > 0 {


in here
we check with ModulesLoadingOrder != nil, in order to be consistent with the code I would change to
cfg.Modprobe.ModulesLoadingOrder != nil

using len is safer

so lets change it here to be consistent

TomerNewman · 2026-07-02T08:55:53Z

/lgtm

TomerNewman · 2026-07-02T08:56:14Z

/unlgtm

TomerNewman · 2026-07-02T08:56:22Z

/remove-lgtm

TomerNewman · 2026-07-02T08:56:33Z

/hold

TomerNewman · 2026-07-02T08:57:00Z

/unhold

I lgtmed accidently

scenario: 1.KMM Modules needs to load module_a, module_b, module_c 2. module_a has a hard symbol dependency on module_b. 3. modulesLoadingOrder defined as: [module_a, module_b, module_c] There is no problems when loading. When unloading using softdep, the following happens 1. module_a is unloaded first 2. module_b is unloaded automatically, due to hard symbol dependency 3. modprobe looks at softdep, sees that module_b is unloaded, and does not continue to module_c This commit changes the Unload flow: 1. no softdep is created. 2. modprobe -r ocmmand is provided with the list of modules defined in the modulesLoadingOrder This way it will go over every module in the command and try to unload it

TomerNewman · 2026-07-02T09:56:31Z

/lgtm

kubernetes-prow Bot requested review from mresvanis and ybettan July 1, 2026 19:45

kubernetes-prow Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 1, 2026

kubernetes-prow Bot assigned TomerNewman and ybettan Jul 1, 2026

TomerNewman reviewed Jul 2, 2026

View reviewed changes

yevgeny-shnaidman force-pushed the yevgeny/fix-softdep-unload branch from e0351cf to 6c423df Compare July 2, 2026 07:15

TomerNewman reviewed Jul 2, 2026

View reviewed changes

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2026

kubernetes-prow Bot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2026

kubernetes-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2026

kubernetes-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2026

yevgeny-shnaidman force-pushed the yevgeny/fix-softdep-unload branch from 6c423df to d5a9547 Compare July 2, 2026 09:01

yevgeny-shnaidman force-pushed the yevgeny/fix-softdep-unload branch from d5a9547 to 9dc420d Compare July 2, 2026 09:54

kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2026

kubernetes-prow Bot merged commit f17d16d into kubernetes-sigs:main Jul 2, 2026
23 checks passed

Uh oh!

Conversation

yevgeny-shnaidman commented Jul 1, 2026

Uh oh!

netlify Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Uh oh!

kubernetes-prow Bot commented Jul 1, 2026

Uh oh!

yevgeny-shnaidman commented Jul 1, 2026

Uh oh!

codecov-commenter commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

TomerNewman left a comment

Choose a reason for hiding this comment

Uh oh!

yevgeny-shnaidman commented Jul 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomerNewman commented Jul 2, 2026

Uh oh!

TomerNewman commented Jul 2, 2026

Uh oh!

TomerNewman commented Jul 2, 2026

Uh oh!

TomerNewman commented Jul 2, 2026

Uh oh!

TomerNewman commented Jul 2, 2026

Uh oh!

TomerNewman commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented Jul 1, 2026 •

edited

Loading

codecov-commenter commented Jul 1, 2026 •

edited

Loading